-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add namespace support for rename operation #12563
Conversation
@jbclements: feedback? |
Be warned that this change comes with cost. Just tried #10607 out:
|
I'm excited to see work done in this area, thank you! That performance regression seems quite worrisome, however, do you know why it's happening? |
(I'm a strong r- on this with the performance regressions, in particular because I think #12512 (comment) would work without a huge hit, even if it's not quite as clean. FWIW, However, without examining the code yet, I imagine you just may have accidentally added a source of O(n^2) (or worse) behaviour. |
Some(ts) => ts.clone() | ||
}; | ||
tables.borrow().with_mut(|ts| | ||
ts.find_or_insert_with(ns, |_| Rc::new(RefCell::new(new_sctable_internal()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why we need a hashmap of two things.
struct SCTables {
lifetime: Rc<RefCell<SCTable>>
plain: Rc<RefCell<SCTable>>
}
should work. (Stored as just SCTables
in TLD.)
(I imagine this is most of the performance problem: hashing 1 bits has a lot of overhead.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll experiment with this. I was anticipating the possible (now seems to be highly unlikely) event of adding a third IdentKind
so that SCTables
must keep synchronised. With hashmap it'll do so automatically. But of course you are right; the performance penalty is just too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you select the field with a match like
match ns {
PlainIdent => ...,
LifetimeIdent => ...,
}
then the addition of a new IdentKind
will cause compilation to fail unless these are updated too (not quite automatic, but not a risk of a bug).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid me :)
Turns out in the middle of recursion, I replaced Also @huonw, I did take your advice #12512 (comment) and added an additional argument to
|
Does this really change it significantly? (If you feel like it, it might be interesting to revert back to the
I don't understand what you mean by this sentence. (Also, it looks like |
Yes, it did. It was the offender and surprisingly,
I meant At the moment, I don't handle the 'real' lifetime folding at all, just the loop labels. |
@@ -651,248 +649,296 @@ pub fn pat_is_ident(pat: @ast::Pat) -> bool { | |||
} | |||
|
|||
// HYGIENE FUNCTIONS | |||
pub mod mtwt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this could move to be either syntax::mtwt
or syntax::ext::mtwt
rather than squirrelled away in this rather mix-and-match "util" module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe syntax::mtwt
, syntax::ext
are mostly macro definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... the whole expansion and hygiene renaming phases happen in syntax::ext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, syntax::ext
it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, while you're at it, adding a doc comment along the lines of
//! Machinery for hygienic macros, as described in the MTWT[1] paper.
//!
//! [1] Matthew Flatt, Ryan Culpepper, David Darais, and Robert Bruce Findler. 2012. *Macros that work together: Compile-time bindings, partial expansion, and definition contexts*. J. Funct. Program. 22, 2 (March 2012), 181-216. DOI=10.1017/S0956796812000093 http://dx.doi.org/10.1017/S0956796812000093
(with more line breaks... ;) )
This seems very reasonable to me (the idea as well as the implementation), but I'd like to just get some confirmation from @jbclements that this isn't going to fundamentally break our macros (or anyone else reasonably familiar with it). |
The mtwt hygiene code moved to
It could come from other (unrelated) patches I pulled in, though :D Seriously, it should have something to do with fixing FIXME 5074, which improves |
table: &SCTable, | ||
resolve_table: &mut ResolveTable) -> Name { | ||
let key = (id.name, id.ctxt); | ||
match resolve_table.contains_key(&key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there would be some speed gains in resolve by writing this as
match resolve_table.find(&key) {
Some(&name) => return name,
None => {}
}
let resolved = { ... };
...
(to only hash key
once; I have a strong feeling that hashing once rather than twice was a major contributor to the speed-up seen on the #10607 smoke-test.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on it. It is exclusively mutably borrowed and recursive so the find_or_insert_with
trick doesn't seem to work right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the early return is as good as you'll do here. :) (And it's certainly the lowest hanging fruit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resolve time was dozen milliseconds so -Z time-passes
is not able to properly gauge speed gains here, not statistically convincingly at least. Anyway, I gonna check this in; the code is much better that way.
Awesome work on the last commit! (Code that's shorter, simpler and faster!) |
Another 2x speed gain by specializing hasher for |
(This needs a rebase too.) |
great work here! 👍 |
Rebased. There was a massive change from |
I'm... surprised by this: theoretically |
fn new_mark_internal(m: Mrk, tail: SyntaxContext, table: &SCTable) -> SyntaxContext { | ||
let key = MarkKey { mrk: m, tail: tail }; | ||
let mut mark_memo = table.mark_memo.borrow_mut(); | ||
let new_ctxt = || idx_push(table.table.borrow_mut().get(), Mark(key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, why is there two layers of closures here? Couldn't this just be inlined in to the |_| ...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say:
let new_ctxt = |_: &(SyntaxContext, Mrk)| ...
*mark_memo.get().find_or_insert_with(key, new_ctxt)
The type inference get choked here :)
Ok, looked over it; there's nothing obvious about why it would be particularly slower. Also, I thought about it a bit, and I'm not so keen on the custom hasher: it does give good speed-ups, but it might be better to wait until the centralised hasher has been optimised some more before we start overriding default type params and generally spreading |
Ok, I removed the last commit about specialising hasher. Regarding the slight performance issue, the vector element in question is a tuple In any case, the performance difference is not that significant. |
I don't believe that is true: (Increasing the capacity by just 1 each time would cause quadratic runtime, due to having to realloc O(n) times.) |
You are right, I should have been more thorough. |
To be absolutely certain, I reverted |
@jbclements comments? |
Aw, geez. This whole thread is something I really have not had the time to keep up with. You finally got my attention, though, so I have at least one big question. Why do you need separate tables for lifetimes? Can you show me a concrete example where having a single table would cause a problem? I believe that the existing setup handles different kinds of bindings pretty transparently. |
Here is the example from the original bug report: fn main() {
let mut foo = ~[];
'foo: for i in [1, 2, 3].iter() {
foo.push(i);
}
} One consequence of adding hygiene label support is that now label |
no... why would label foo shadow the let binding? One's a binding, one's a lifetime. The lifetime shouldn't end up in an environment during name resolution. I'm worried that something is seriously wrong here. |
Loop labels are lifetime at token level, but they become |
@edwardw Grr... okay, gears grinding. You may be right. Lemme think about it more. |
I split general improvements over the |
Maintains two renaming tables, one for regular idents and one for lifetime idents (a loop label is of lifetime kind) so that let bindings and loop labels with same name won't clash. Closes rust-lang#12512.
Closing due to inactivity. It sounds like the way to solve #12512 has a few options, and the solution may need to get settled on before making more progress. It seems like a serious bug though, and I'd love to see it fixed! |
internal: Simplify
make sure checked type implements `Try` trait when linting [`question_mark`] (indirectly) fixes: rust-lang#12412 and fixes: rust-lang#11983 --- changelog: make sure checked type implements `Try` trait when linting [`question_mark`]
Maintains two renaming tables, one for regular idents and one for
lifetime idents (a loop label is of lifetime kind) so that let bindings
and loop labels with same name won't clash. Also, the mtwt hygiene code
is now @-free.
Closes #12512.